fix(cli): tools preference and catalog exit with a message on bad input or output - #239

Review at 192057b. The fix does what it says for the cases it names: non-UTF-8 --groups, unwritable --items-out and --out, unreadable or non-UTF-8 manifests all exit with a one-line message. The problems are on the paths right next to them, which still end in a traceback, and in one test that does not pin the widened except. I'd fix these before merge.

What I ran, on a Linux review host at 192057b:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: with external/catalog.py or commands/catalog.py reverted to the base, the new catalog tests fail (4 and 3 tests). Seven of eight mutations of the tools code and tests are caught; the survivor is the first inline test comment on test_tool_preference.py.

Not anchored inline:

1. tools preference --out-json is still unguarded, and it is written after all the model calls. tools.py:139 calls save_artifact("tool_preference", data, args.out_json). With --out-json blocker/o.json, where blocker is a regular file, the command ends in a FileExistsError traceback after the paid calls have run. --items-out is now checked before any spend, so this is the remaining unguarded output. This predates the PR, but the PR title and changelog say tools preference exits with a message on an unwritable output. Suggestion: guard it the same way as _write_items, or check the parent of --out-json before the calls.

Checked, no issues

• except ValueError in tools.py: Copilot's point on the broad ValueError does not apply: tp.GroupsError is a ValueError subclass, and I found no input that makes load_groups raise another ValueError (wrong types for ids, names and vendors all become GroupsError). The only other ValueError in the module is raised from build_items, outside the try.
• Callers of the manifest readers: read_skill_manifest and read_task_manifest are called only from the two scan helpers, and both catch ValueError.
• --out failures: a directory, a file as parent, a symlink loop and a dangling symlink all give the one-line message for reach and skill-creator, and for questions apart from the empty path below.
• Repo rules: the changelog fragment passes the fragment check, and the growth baseline entry (16 to 15) is correct per check_growth.py. No schema or docs pages are touched.

I pushed a commit per nit, up to 8940627, which fix the two nits from my review:

• 766eae4: dokimi/dokimi/commands/catalog.py:140: the docstring now ends "when it cannot be written."
• 8940627: dokimi/tests/test_skill_selection.py:485: the match= pattern wraps the path in re.escape(str(out)); import re was already there.

Left as is:

• dokimi/dokimi/commands/tools.py:139: the unguarded --out-json is not labelled Nit: or Non-blocking:, and guarding it would change behaviour, so it stays with the author.

make ci-local at 8940627: 29 passed, 0 failed, 7 skipped.

Not approving yet. Still open: the five risks from my review.

The change wraps the reads and writes of tools preference, catalog trigger-set and catalog scan so OS and decode errors exit with a one-line message. It also validates the shape of suites.json and of the groups document. The changes look correct and the tests cover the new paths. I found one minor problem with the message wording. I could not run the tests here because pytest isn't installed in the container.

1 finding is on the changed lines, as comments below.

Not verified

I did not run the new tests. I also did not check that make ci-local, including the growth baseline drop from 16 to 15 for cmd_catalog_trigger_set, is green.

Re-review of 5d1c627fe3d8: 1 new finding.

1 finding is on the changed lines, as comments below.

Review at 5d1c627f.

Summary. All earlier findings (antonwolfy, Copilot, exai-bot round 1) are addressed in 0f9bfd9 / 766eae4 / 8940627 / 5d1c627: wrong-shape suites.json and --groups, the narrowed except with a test that pins it, --out "", and the schema-rejection message. I found nothing new on the changed lines and pushed no commits.

Open items

• Minor, design choice for the author (not a small fix, so left as is): tools preference only discovers an unwritable --out-json in _save_result, after the paid probe run, so the results are lost (exai-bot at tools.py:149, also mentioned by antonwolfy as left to the author). A cheap pre-flight (e.g. parent.mkdir + open-for-append check before run_probe) would avoid paying for a run that cannot be saved. Pre-existing behaviour; the PR adds the clean message but not the early check. OK as a follow-up.

Test evidence (Linux, uv run)

• ruff check dokimi tests: clean.
• test_catalog_command.py test_skill_selection.py test_tool_preference.py test_tools_preference_items_out.py: 147 passed.

Verdict from my side: ready; the --out-json pre-flight can be a follow-up.

Review at 53e5e33. Both fixes from my last review hold: "groups": null and --out-json "" now exit with a message. Two things are worth fixing before merge: the same deep-nesting input that was fixed for --groups still ends in a traceback in the manifest readers, and the new tool_preference artifact rejected branch has no test.

What I ran, on a Linux review host at 53e5e33:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: dropping the ArtifactValidationError branch in _save_result leaves test_tool_preference.py and test_tools_preference_items_out.py green (77 passed). Reverting the groups: null, deep-nesting and huge-int fixes makes the new parametrized cases fail.

@napetrov's and @exai-bot's question on checking --out-json before the paid run still waits for the author. The command now exits with a message, but the results are still lost when the path is unwritable.

Checked, no issues

• --groups: about 50 inputs (nested wrong types, BOM, UTF-16, empty file, NaN, a directory, a missing path, a 5000-character path, 6000-digit ints, [ and {"a": nested deeply) all exit with --groups <path>: ....
• --out-json: an empty path, a directory, a file as parent and . all exit 1 with Error: cannot write <path>: ....
• --out in trigger-set: three formats against 19 odd paths (directory, file as parent, empty, symlink loop, /dev/null/x, 300-character name) give a one-line message, never a traceback.
• Re-indented loop: git diff -w shows only the new _manifest_tasks call in the loop header.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

Delta review of 5d1c627f..53e5e33b.

New commit 53e5e33 (null groups, deep/huge JSON in --groups, empty --out-json, schema rejection reported separately from an unwritable path): reads correct; ArtifactValidationError is caught before the broader ValueError, so the two messages stay distinct. Nothing new from me, no commits pushed.

Still open (unchanged, follow-up OK): --out-json is only checked after the paid probe run.

Evidence: ruff check dokimi tests clean; test_tool_preference.py test_tools_preference_items_out.py 77 passed; CI 20/20 green at 53e5e33.

Review at 6c4295c. Both fixes from my last review hold: a deeply nested skills.yaml or suites.json now exits with an invalid ... at <path> message, and the tool_preference artifact rejected branch has a test that fails when the clause is removed. Two risks remain on the neighbouring catalog paths, which still end in a traceback. Both predate the PR, but the changelog says a bad manifest exits with a message.

What I ran, on a Linux review host at 6c4295c:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: reverting the RecursionError catch for skills.yaml, the one for suites.json, and the OSError handler around is_task_dir makes exactly one new test fail each. Removing the except ArtifactValidationError clause in _save_result makes the new tools test fail with cannot write.

Not anchored inline (outside this diff):

1. A malformed SKILL.md under --catalog still ends in a traceback in catalog trigger-set. discover_skills(catalog_root) at commands/catalog.py:171 runs outside any try. With --catalog holding a/SKILL.md and a valid b/SKILL.md, and --questions q.json=b --out o.json: bytes \xff\xfe give UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff; frontmatter name: [ or a YAML list gives ValueError: Skill 'a' (...) is missing a 'description' in its frontmatter; frontmatter of [ repeated 100000 times gives RecursionError; a/SKILL.md as a directory gives IsADirectoryError. All exit 1 and write no --out. catalog scan reports the same inputs per row, so the two commands disagree. Suggestion: wrap the call in try/except (OSError, ValueError, RecursionError) and raise SystemExit(f"--catalog {catalog_root}: {exc}"), with a parametrized test for a non-UTF-8 and a missing-description SKILL.md.
2. catalog scan --kind tasks without a suites.json still ends in a traceback on a task directory with no instruction.md. discover_tasks(tasks_root) at external/catalog.py:273 is unguarded, unlike the manifest branch below it. With evaluation/harbor/tasks/t/task.toml present, no instruction.md and no suites.json, catalog scan --root <dir> --kind tasks exits with FileNotFoundError: Task has no instruction.md: .../tasks/t. The same task listed in suites.json gives a failed row. Suggestion: catch (OSError, ValueError) around discover_tasks, raise ValueError(f"cannot read tasks at {tasks_root}: {exc}") (which commands/catalog.py already turns into a message), and add a test with a task directory holding only task.toml.

@napetrov's and @exai-bot's question on checking --out-json before the paid run still waits for the author. The command exits with a message, but the results are still lost when the path is unwritable.

Checked, no issues

• Over-long task name: with the tasks directory present, a 300-character track: executable name gives a failed entry (cannot inspect task directory ...: File name too long) in text and JSON, exit 1. A 255-character name is skipped, exit 0.
• Deep nesting: [ repeated 200000 times in skills.yaml (--kind skills and all) and in suites.json (--kind tasks and all) exits with the invalid-manifest message.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

Review at 61cdeb2. Both fixes from my last review hold: catalog trigger-set exits with a message on a malformed SKILL.md, and catalog scan --kind tasks does the same on a task directory without instruction.md. A sibling path in catalog scan still ends in a traceback for the nested-frontmatter input the new code handles, and the new tests do not pin two of the five new exception types. There is also one wording nit.

What I ran, on a Linux review host at 61cdeb2:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code findings, offline.
• Mutation runs: reverting either call site makes the matching new test fail. Dropping RecursionError from the _discover_catalog_skills tuple, or OSError (a SKILL.md that is a directory), leaves the 23 tests in test_catalog_command.py green.

Not anchored inline:

1. Nit: the task-directory error is prefixed suites.json: when no suites.json exists. _scan_tasks (commands/catalog.py:105, outside this diff) wraps every ValueError from read_task_manifest as suites.json: {exc}. The new _discover_tasks_checked runs only in the branch where the manifest is absent. A task directory holding only task.toml gives suites.json: cannot read tasks at <root>/evaluation/harbor/tasks: Task has no instruction.md: <root>/evaluation/harbor/tasks/t. Suggestion: drop the suites.json: prefix, since the invalid suites.json at <path> messages already name the file.

@napetrov's and @exai-bot's question on checking --out-json before the paid run waits for the author.

Checked, no issues

• trigger-set --catalog: a SKILL.md that is a directory, a symlink loop, mode 0, a list frontmatter and a 100000-deep frontmatter all exit 1 with --catalog <root>: ....
• read_task_manifest success path: hoisting found out of the comprehension leaves a good task plus an empty skills directory printing the same ok row.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

Review at 263ccde. Both fixes from my last review hold: a deeply nested SKILL.md is now an error row in catalog scan, the new tests pin each added exception, and the suites.json: prefix is gone from the task-directory message. One thing is worth fixing before merge: the new --out-json pre-flight in tools preference does not match what the real save does.

What I ran, on a Linux review host at 263ccde:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). The changed-line coverage check passed.
• Reproductions for the code finding, offline.
• Mutation runs: dropping RecursionError from external/catalog.py, from external/validate.py or from commands/catalog.py makes exactly one new test fail each.

Checked, no issues

• Prefix removal: no test or doc still expects suites.json: in front of the task-directory error.
• Deep SKILL.md: catalog scan --kind skills on name: followed by 100000 [ prints an ERROR: row plus a frontmatter-parse finding, exits 1, and the other skills still scan.
• Pre-flight order: --dry-run and a missing --model exit before the check, and a missing target file is removed again.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

The --out-json probe now matches the save in the cases I tried, and make ci-local is green. One thing is worth fixing before merge: the tests do not pin the new probe.

What I ran, on a Linux review host at ad114ec:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). 8,264 tests; changed-line coverage 97%.
• Reproductions for the probe against the real save, offline.
• Mutation runs: 8 mutants of _check_out_json_writable; 3 are caught (no _write_target, no is_dir check, no mkdir) and 5 pass the full suite (no probe, probing target.parent, is_dir on target, no ValueError in the except, and a changed temp prefix). The first three are in the inline comment; the ValueError one needs an embedded NUL byte in the path, which a shell cannot pass, and the prefix one is cosmetic.

Checked, no issues

• Probe against save: a read-only file in a writable directory, a symlink to one, a dangling symlink, a symlink into a missing directory, a symlink loop, a directory target and an empty path give the same accept or reject in the check and in save_artifact. A dangling symlink leaves no file at its target, and no .tmp file is left behind.
• Call order: _check_out_json_writable runs after --dry-run and the --model check and before the first probe call.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

Review at 41466dd. The three tests from my last review are in, and they kill the probe mutants I named (no probe, target.parent, no is_dir on the resolved path). One nit inline: the probe's temp name is shorter than the save's, so a very long file name passes the pre-flight and the save then fails after the paid calls.

What I ran, on a Linux review host at 41466dd:

• make ci-local: 29 passed, 0 failed, 7 skipped (docker, network and other-Python legs). 8,267 tests; changed-line coverage 97%.
• Reproduction for the code finding, offline.
• Mutation runs: of 7 mutants of _check_out_json_writable, 6 are caught (no probe, target.parent, no mkdir, no is_dir check, open(..., 'ab') probe, no _write_target). Dropping ValueError from the except passes the suite; it needs an embedded NUL byte in the path, which a shell cannot pass, so I did not raise it.

Checked, no issues

• Probe against save: a read-only file, a symlink into a read-only directory, a dangling symlink, a symlink loop (as file and as parent), a trailing slash, an empty path and a 0555 directory give the same accept or reject in the check and in save_artifact, and the probe leaves no file behind.
• Call order: the check runs after --dry-run and the --model check, and before the first model call.
• Repo rules: the changelog fragment and the growth baseline (16 to 15) pass; no schema or docs pages are touched.

I pushed a commit per nit, up to 8f04440, which fix the nit from my review:

• 8f04440: dokimi/dokimi/commands/tools.py:129: the probe now creates <name>.<pid>.<thread id>.tmp, the save's temp name, and a new test pins a 240-character file name to exit 2.

make ci-local at 8f04440: 29 passed, 0 failed, 7 skipped.

GitHub CI has not run on these commits: the branch conflicts with main, and Actions runs no workflow on a conflicting PR. CI will check them once main is merged in.

Not approving until CI passes at this head.

LGTM. Both new commits are right: a SKILL.md that is too deeply nested and is listed in skills.yaml is now an error row, and suites.json rejects {}, 0 and {"tasks": {}} while null or a missing key still reads as empty. No new findings. @xaleryb left one choice to you: whether to turn RecursionError into ValueError once, in load_skill.

What I ran, on Linux at 3d29187:

• make ci-local: 26 passed, 2 failed, 7 skipped; 8,722 tests; the changed-line coverage check passed. The two failures are the gitleaks install and scan steps, which could not download the gitleaks binary (curl: (22) ... 504); not caused by this PR.
• Mutation runs on the new tests: dropping RecursionError from _skill_entry, restoring or [] for suites or for tasks, and removing either None to [] mapping each make at least one new test fail.

Checked, no issues

• Delta: catalog scan --kind skills on a skills.yaml that lists a SKILL.md with [ repeated 100000 times in the frontmatter prints an ERROR: row, exits 1, and the other skills still scan.
• Repo rules: the changelog fragment is present, the growth baseline entry (16 to 15) passes, and no schema is touched.